-
Notifications
You must be signed in to change notification settings - Fork 545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Implement 'env' package to handle environment variables in cosign #2322
Conversation
cc @znewman01 PTAL ^^ 🙂 |
Codecov Report
@@ Coverage Diff @@
## main #2322 +/- ##
==========================================
+ Coverage 29.85% 30.11% +0.26%
==========================================
Files 134 136 +2
Lines 8287 8344 +57
==========================================
+ Hits 2474 2513 +39
- Misses 5482 5498 +16
- Partials 331 333 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I have an issue with running
Any idea how can I fix this? I'm running Ubuntu 22.04. |
you need to install the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea to redact sensitive information by default 👍
@znewman01 I should have addressed all your comments. I'm trying to configure |
@znewman01 I added
Please take a look at this and let me know if this approach is okay or if I should change anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: that was the wrong PR, sorry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really great first contribution to Cosign, thank you so much!
Some changes requested, mostly minor.
@znewman01 Thank you so much for the review! I agree with all those comments. It’s already late for me (I’m in the CEST time zone), so I’ll address those comments beginning of the next week if that’s okay. 🙂 |
No rush! Have a great weekend |
Signed-off-by: Marko Mudrinić <[email protected]>
Signed-off-by: Marko Mudrinić <[email protected]>
Signed-off-by: Marko Mudrinić <[email protected]>
Signed-off-by: Marko Mudrinić <[email protected]>
Signed-off-by: Marko Mudrinić <[email protected]>
Signed-off-by: Marko Mudrinić <[email protected]>
Signed-off-by: Marko Mudrinić <[email protected]>
Signed-off-by: Marko Mudrinić <[email protected]>
Signed-off-by: Marko Mudrinić <[email protected]>
@znewman01 I think that I should have addressed all comments. Please let me know if I forgot something or if you have any additional comments. I tried to document all variables as best as I could, but I'm not sure have I got all descriptions and expectations correctly, so please take a look at that. |
Signed-off-by: Marko Mudrinić <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So close! This is looking great. Just a couple small things
Signed-off-by: Marko Mudrinić <[email protected]>
Signed-off-by: Marko Mudrinić <[email protected]>
@znewman01 I addressed your comments, PTAL when you have some time. 🙂 |
Small lint issue, once you fix that I think we can merge! |
Signed-off-by: Marko Mudrinić <[email protected]>
@znewman01 Fixed. 🙂 |
Nice, if tests pass this should merge automatically! Thank you so much! 🙏 Great first PR, I wish everybody sent one like this 😄 |
@znewman01 I'm so happy to see this PR merged! Thanks a lot for going through this with me, I really appreciate it! |
These were removed in sigstore#2322, which made it an accidentally-semver-breaking change. This unbreaks semver. These should be removed at the next major version release or sooner if we decide keeping semver is not important. Signed-off-by: Zachary Newman <[email protected]>
These were removed in sigstore#2322, which made it an accidentally-semver-breaking change. This unbreaks semver. These should be removed at the next major version release or sooner if we decide keeping semver is not important. Signed-off-by: Zachary Newman <[email protected]>
These were removed in #2322, which made it an accidentally-semver-breaking change. This unbreaks semver. These should be removed at the next major version release or sooner if we decide keeping semver is not important. Signed-off-by: Zachary Newman <[email protected]> Signed-off-by: Zachary Newman <[email protected]>
Summary
This PR implements a new
env
package to handle environment variables in cosign. This PR/implementation targets onlyCOSIGN_
environment variables, but it can be extended to handle other environment variables as well. This implementation is based on hard mode described here: #2236 (comment)The
env
package implements:Variable
type which is used to define an environment variableVariableOpts
type which is used to describe an environment variableGetenv(Variable) string
function which is a wrapper aroundos.Getenv
for givenVariable
LookupEnv(Variable) (string, bool)
function which is a wrapper aroundos.LookupEnv
for givenVariable
PrintEnv()
function which is used to print all known variables, their values, and descriptionsAll
os.Getenv
/os.LookupEnv
calls onCOSIGN_
variables are replaced withenv.Getenv
andenv.LookupEnv
. There are some otheros.Getenv
calls on non-cosign variables, but those will be handled in a follow up PR (if this turns out to be an approach we want to follow). The purpose of this change is that we want to forbid usingos.Getenv
-related function and require usingenv
package. That combined withVariable
type ensures that each environment variable used by cosign must be registered in theenv
package.This PR also modifies
cosign env
command with some additional features. First of all, the command is now usingenv.PrintEnv()
to print the known cosign variables instead of searching Environ. However, we still search Environ forSIGSTORE_
andTUF_
variables. Additionally, there are two new flags:--show-descriptions
(defaulttrue
) -- used to enable descriptions for known cosign variables--show-sensitive-values
(defaultfalse
) -- by default,cosign env
is going to show asterisk signs if variable is marked as sensitive. Turning this flag on will show values for sensitive variablesExample 1 (with descriptions and hidden sensitive values, default):
Example 2 (with sensitive values):
This PR integrates
forbidigo
linter to forbid usingos.Getenv
andos.LookupEnv
. We want to enforce theenv
package to be used so that we can register all environment variables. There are some legitimate use cases where we still useos.Getenv
andos.LookupEnv
, such as in tests or with non-cosign environment variables. I handled ignoring those legitimate use cases in two ways:os.Getenv
/os.LookupEnv
calls, ignore that file or package in.golangci.yml
fileos.Getenv
/os.LookupEnv
calls, use in-line//nolint:forbidigo
commentThere are other environment variables such as
SIGSTORE_
andTUF_
that are not handled, but this is something that can be discussed and handled later. I believe this PR should be a solid foundation for any further work on this topic.Fixes #2236
Release Note
env
package used to register and handle environment variables used by cosigncosign env
now prints all registeredCOSIGN_
environment variables--show-descriptions
(defaulttrue
) flag tocosign env
used to show description for each registeredCOSIGN_
environment variable--show-sensitive-values
(defaultfalse
) flag tocosign env
used to hidh values forCOSIGN_
environment variables marked as sensitiveDocumentation
I'm not sure, but I guess mostly like yes.